Accept duplicate package manifest fields - #718

Copilot wasn't able to review any files in this pull request.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

ℹ️ The fallback itself is sound. One scope gap and a hot-path nit worth a look.

Reviewed changes — the single-commit diff against main, plus the surrounding aube-manifest parse machinery and every other site that deserializes a package.json.

• Duplicate-key fallback in PackageJson::parse — on any parse_json failure, re-parse through serde_json::Value and from_value, returning the original error if either step fails.
• Test for last-value semantics — duplicate name, scripts, and dependencies keys, asserting the whole nested object is replaced rather than merged.

Last-wins is correct here for a non-obvious reason worth recording: the aube workspace enables serde_json's preserve_order (vendor/aube/Cargo.toml:62), so Map is IndexMap-backed and insert replaces the value while keeping the original key position. Nested Value subtrees whose order is load-bearing (exports conditions, most obviously) round-trip unchanged, so there is no ordering regression on the fallback path. The widened acceptance is also narrow in the right way: JSONC comments, trailing content, NaN/Infinity, and bad escapes all fail from_str::<serde_json::Value> too, so the original diagnostic — including the special "cannot contain JSON comments" message — still surfaces.

ℹ️ Duplicate keys still hard-fail for git:, file:, and tarball-URL dependencies

PackageJson::parse is not the only door into PackageJson. Four sites in aube-resolver deserialize manifest bytes straight into the struct with sonic_rs::from_slice/serde_json::from_slice, so a dependency resolved from a git clone, a file:/link:/portal: path, a yarn exec: manifest, or a bare tarball URL still fails with the same duplicate-field error this PR fixes for registry packages. Placing the fallback inside the generic parse_json instead of PackageJson::parse would close the class rather than the instance.

# Duplicate-key tolerance is applied at one call site, not to the parse layer

## Affected sites
- `vendor/aube/crates/aube-manifest/src/lib.rs:390` — the fallback lives in `PackageJson::parse`, so only
  callers of `parse` / `from_path` / `from_path_cached` benefit.
- `vendor/aube/crates/aube-resolver/src/local_source.rs:169` — `read_local_manifest`, `file:`/`link:`/`portal:`
  dependencies: `sonic_rs::from_slice(&content).or_else(|_| serde_json::from_slice(&content))`.
- `vendor/aube/crates/aube-resolver/src/local_source.rs:243` — `resolve_exec_manifest`, yarn `exec:` manifests.
- `vendor/aube/crates/aube-resolver/src/local_source.rs:387` — `read_git_package_manifest`, git dependencies.
- `vendor/aube/crates/aube-resolver/src/local_source.rs:676` — `resolve_remote_tarball`, bare `https://….tgz`
  dependencies; this one has no `sonic_rs` → `serde_json` retry at all.

## Required outcome
- A `package.json` with duplicate top-level keys parses identically regardless of which source kind it came
  from, so `git+https://…/lzma-native` behaves the same as `lzma-native` from the registry.

## Suggested approach (optional)
- Move the `serde_json::Value` retry into `parse_json` (it is already generic over
  `T: DeserializeOwned`, so the retry is generic too), leave `PackageJson::parse` as a thin delegate, and
  route the four `local_source.rs` sites through `parse_json` so they also pick up the BOM strip and the
  JSONC diagnostic they currently lack.

## Open questions for the human
- Is closing the whole class in scope for this PR, or is the registry path the only one you want moved now?

ℹ️ Nitpicks

• The BOM strip at line 398 duplicates the one inside parse_json (line 1376) and is the only thing making a BOM-prefixed, duplicate-keyed manifest work — no fixture covers that combination.
• Nothing pins the "preserve the original diagnostic" claim from the PR description. A manifest with a // comment asserting the cannot contain JSON comments message still comes back would guard against a future fallback that swallows it.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

ℹ️ No critical issues — one regression-test suggestion inline.

Reviewed changes — reviewed the expanded manifest parser and consumer updates since the prior Pullfrog review.

• Removed the success-path copy — introduced a borrowing JSON parser so ordinary manifest parsing allocates error content only after failure.
• Unified byte-oriented manifest parsing — added PackageJson::from_slice and routed local, git, exec, remote-tarball, ignored-build, and publish manifest reads through the duplicate-key-tolerant path.
• Added failure-diagnostic coverage — added a malformed duplicate-key fixture intended to preserve the original typed-parser diagnostic.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

✅ No new issues found.

Reviewed changes — reviewed the diagnostic regression-test update since the prior Pullfrog review.

• Pinned original-diagnostic behavior — Replaced the malformed fixture with valid duplicate-key JSON whose retry fails on a later type error, then compared the returned message and span with the typed parser's original diagnostic.

AI-assisted — Tool: Pullfrog; model: openai/gpt-sol; version: 0.1.68.

  ^{｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}